Strengthen handling of unavailable cgroup stats by jasontedor · Pull Request #21094 · elastic/elasticsearch

jasontedor · 2016-10-24T14:51:35Z

On some systems, cgroups will be available but not configured. And in
some cases, cgroups will be configured, but not for the subsystems that
we are expecting (e.g., cpu and cpuacct). This commit strengthens the
handling of cgroup stats on such systems.

Relates #21029

s1monw · 2016-10-24T15:00:45Z

should we just throw FNF and catch on a top level as we do in other cases?

On some systems, cgroups will be available but not configured. And in some cases, cgroups will be configured, but not for the subsystems that we are expecting (e.g., cpu and cpuacct). This commit strengthens the handling of cgroup stats on such systems.

jasontedor · 2016-10-24T15:03:21Z

should we just throw FNF and catch on a top level as we do in other cases?

@s1monw We can throw if /proc/self/cgroup is not available, which we do now, but we shouldn't throw if some /sys/fs/cgroup... is not available because it might be the case that the cpu subsystem is configured but not the cpuacct subsystem. Doing it the way that I've done it at least lets us get what is available.

s1monw · 2016-10-24T15:08:09Z

@jason ok can we check this higher up to skip the entire query of this subsystem

jasontedor · 2016-10-24T15:13:02Z

ok can we check this higher up to skip the entire query of this subsystem

Sorry, I wasn't completely clear in my previous comment. We do already check if the subsystem is available or not, but it might still be the case that the necessary mounts to /sys/fs/cgroup/ are not available and this can vary between subsystems (whether or not they are configured, whether or not the mounts to /sys/fs/cgroup are available).

This commit checks for the existence of cgroup stats upfront rather than allowing leniency in the cgroup methods.

jasontedor · 2016-10-24T15:59:49Z

@s1monw I pushed 29ed3ba which might be more to your tastes? 😄

s1monw · 2016-10-24T16:06:00Z

looks great I would be even happier if we had a static method that says boolean isCgroupsAvailable() and add the empty check back?

s1monw · 2016-10-24T18:16:32Z

@jasontedor I am ok with pushing this since some builds are failing but I think we can do better with a nice util method?

This commit refactors the logic for the upfront checking whether the relevant cgroup stats are available or not, and adds a note to the docs.

jasontedor · 2016-10-24T18:31:48Z

I am ok with pushing this since some builds are failing but I think we can do better with a nice util method?

I pushed 4c8a2cc.

This commit adds Javadocs for a convenience method OsProbe#areCgroupStatsAvailable.

s1monw

left some questions, LGTM otherwise

s1monw · 2016-10-24T19:46:25Z

-            final String[] controllers = matcher.group(1).split(",");
-            for (final String controller : controllers) {
-                controllerMap.put(controller, matcher.group(2));
+        if (lines.isEmpty()) {


isn't this obsolete now? we don't return empty form this method?

I pushed 8c7d7be.

s1monw · 2016-10-24T19:46:36Z

+     * @return {@code true} if the stats are available, otherwise
+     * {@code false}
+     */
+    @SuppressForbidden(reason = "access /proc/self/cgroup, /sys/fs/cgroup/cpu, and /sys/fs/cgroup/cpuacct")


s1monw · 2016-10-24T19:46:52Z

-                getCgroupCpuAcctCpuCfsPeriodMicros(cpuControlGroup),
-                getCGroupCpuAcctCpuCfsQuotaMicros(cpuControlGroup),
-                getCgroupCpuAcctCpuStat(cpuControlGroup));
+            if (!areCgroupStatsAvailable()) {


s1monw · 2016-10-24T19:47:49Z

        @Override
        public void writeTo(final StreamOutput out) throws IOException {
-            out.writeString(cpuAcctControlGroup);
+            out.writeOptionalString(cpuAcctControlGroup);


you require it non-null above but here it's optional?

That was a bug, I cleaned up all the optionality in 4c8a2cc but missed this one. I pushed 1d53577.

This commit removes some leniency when reading cgroups that was leftover after a previous refactoring.

This commit fixes a serialization bug in OsStats when serializing Cgroup instances. Namely, the control groups were made non-optional in a previous refactoring but the commit that cleaned this up left one case of writing a string as optional.

jasontedor · 2016-10-24T19:57:28Z

Thanks @s1monw, I've responded to your feedback.

s1monw · 2016-10-24T20:29:40Z

LGTM 👍

On some systems, cgroups will be available but not configured. And in some cases, cgroups will be configured, but not for the subsystems that we are expecting (e.g., cpu and cpuacct). This commit strengthens the handling of cgroup stats on such systems. Relates #21094

jasontedor · 2016-10-24T20:39:38Z

Thanks @s1monw.

jasontedor added review :Core/Infra/Stats Statistics tracking and retrieval APIs v6.0.0-alpha1 v5.1.1 labels Oct 24, 2016

jasontedor changed the title ~~Strength handling of unavailable cgroup stats~~ Strengthen handling of unavailable cgroup stats Oct 24, 2016

Check for existence of cgroup stats upfront

29ed3ba

This commit checks for the existence of cgroup stats upfront rather than allowing leniency in the cgroup methods.

Make explicit upfront checking for cgroup stats

4c8a2cc

This commit refactors the logic for the upfront checking whether the relevant cgroup stats are available or not, and adds a note to the docs.

Add Javadocs for OsProbe#areCgroupStatsAvailable

c322b76

This commit adds Javadocs for a convenience method OsProbe#areCgroupStatsAvailable.

s1monw reviewed Oct 24, 2016

View reviewed changes

jasontedor added 2 commits October 24, 2016 15:53

Remove obsolete leniency when reading cgroups

8c7d7be

This commit removes some leniency when reading cgroups that was leftover after a previous refactoring.

Fix serialization bug in OsStats

1d53577

This commit fixes a serialization bug in OsStats when serializing Cgroup instances. Namely, the control groups were made non-optional in a previous refactoring but the commit that cleaned this up left one case of writing a string as optional.

jasontedor merged commit 900ee05 into elastic:master Oct 24, 2016

jasontedor deleted the unavailable-cgroup branch October 24, 2016 20:39

clintongormley added the >enhancement label Nov 5, 2016

Conversation

jasontedor commented Oct 24, 2016

Uh oh!

s1monw commented Oct 24, 2016

Uh oh!

jasontedor commented Oct 24, 2016

Uh oh!

s1monw commented Oct 24, 2016

Uh oh!

jasontedor commented Oct 24, 2016

Uh oh!

jasontedor commented Oct 24, 2016

Uh oh!

s1monw commented Oct 24, 2016

Uh oh!

s1monw commented Oct 24, 2016

Uh oh!

jasontedor commented Oct 24, 2016

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

s1monw Oct 24, 2016

Choose a reason for hiding this comment

Uh oh!

jasontedor Oct 24, 2016

Choose a reason for hiding this comment

Uh oh!

s1monw Oct 24, 2016

Choose a reason for hiding this comment

Uh oh!

s1monw Oct 24, 2016

Choose a reason for hiding this comment

Uh oh!

s1monw Oct 24, 2016

Choose a reason for hiding this comment

Uh oh!

jasontedor Oct 24, 2016

Choose a reason for hiding this comment

Uh oh!

jasontedor commented Oct 24, 2016

Uh oh!

s1monw commented Oct 24, 2016

Uh oh!

jasontedor commented Oct 24, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants